Code Review: feat(tokenless): add MCP tokenless_retrieve server

This PR adds a minimal MCP stdio server (tokenless mcp serve) that exposes tokenless_retrieve so an MCP-connected agent can recover stashed payloads on demand. The implementation is hand-rolled JSON-RPC (no SDK dependency), deliberately minimal in scope (one tool),
and reuses the existing open_stash_store_or_err + extract_hash infrastructure from #1270. The test coverage (6 tests) covers tool listing, dispatch errors, hash validation, round-trip retrieve, marker-line input, and missing-payload errors.

Findings

1. 🟠 SQLite store opened per-request — wasteful for a long-running server

src/tokenless/crates/tokenless-cli/src/mcp.rs — retrieve() function

retrieve() calls open_stash_store_or_err(None) on every tools/call request. SqliteStore::new() runs Connection::open, three PRAGMAs (journal_mode=WAL, busy_timeout=5000, synchronous=NORMAL), CREATE TABLE IF NOT EXISTS, and CREATE INDEX IF NOT EXISTS — five SQL
statements before any actual retrieve query. For a long-running MCP server handling many retrieval requests in a session, this creates and tears down a SQLite connection per request.

SqliteStore is designed for shared long-lived use (wraps Connection in Mutex with poison-recovery), so creating one per request defeats its design.

Suggested fix: Open the store once at the top of serve() (or lazily on first tools/call) and pass it into retrieve() / retrieve_from_store():

pub fn serve() -> Result<(), (String, i32)> {
let store = open_stash_store_or_err(None).ok(); // fail-open at startup
// ... loop ...
"tools/call" => ok(id, handle_tool_call(params, &store)),
// ...
}

2. 🟠 Malformed JSON silently dropped — violates JSON-RPC 2.0

src/tokenless/crates/tokenless-cli/src/mcp.rs:32-34

Err(_) => {
continue; // Skip unparseable lines
}

JSON-RPC 2.0 Section 5 requires a Parse Error response: {"jsonrpc":"2.0","id":null,"error":{"code":-32700,"message":"Parse error"}}. If a client sends syntactically broken JSON that it expects a reply to, it will hang indefinitely waiting for a response that never
comes.

The comment says "some clients prefix LSP-style headers" — but those lines are typically Content-Length: N\r\n which would fail JSON parsing. A better approach: check if the line looks like a header (starts with a letter, contains :) before attempting JSON parse, and
only send -32700 for lines that look like JSON but fail to parse.

Suggested fix: Send a -32700 error on parse failure:

Err(_) => {
let _ = writeln!(out, "{}", err(Value::Null, -32700, "Parse error"));
let _ = out.flush();
continue;
}

3. 🟡 Missing ping handler — MCP clients may mark server as unhealthy

src/tokenless/crates/tokenless-cli/src/mcp.rs:40-57

MCP 2024-11-05 defines ping as a standard utility method that either side can send to verify liveness, expecting an empty result {}. The current code falls through to the other arm and returns -32601 method not found: ping. MCP clients (like Claude Desktop) that send
periodic health-check pings would receive a protocol error, potentially marking the server as unhealthy or disconnecting.

Suggested fix: Add "ping" => ok(id, json!({})) to the method match.

4. 🟡 No input size limit on stdin — unbounded memory allocation

src/tokenless/crates/tokenless-cli/src/mcp.rs:20

BufRead::read_line appends to the String buffer without any size limit. A malicious or buggy client could send a single JSON line containing gigabytes of data, causing OOM. The main CLI already guards against this with MAX_INPUT_BYTES (64 MiB) via Read::take. The MCP
server has no equivalent protection.

Suggested fix: Check line.len() after each read_line and reject oversized messages, or use a capped reader.

5. 🟡 tools/call and tools/list accepted before initialize handshake

src/tokenless/crates/tokenless-cli/src/mcp.rs:40-57

MCP requires the initialize → notifications/initialized handshake before other methods are available. The server handles tools/list and tools/call at any time, even before initialize. A well-behaved client will always initialize first, but the server should track
initialization state and return -32002 ("Server not initialized") for premature requests.

Suggested fix: Add a let mut initialized = false; flag, set it on initialize, and gate tools/list and tools/call behind it.

6. 🔵 Duplicate extract_hash call — validation result discarded then recomputed

src/tokenless/crates/tokenless-cli/src/mcp.rs

retrieve() calls extract_hash(hash) to validate the format (checking .is_none()), discards the result, then retrieve_from_store() calls extract_hash(hash) again to actually extract the key. The string scanning work is done twice per request.

Suggested fix: Extract once in retrieve(), pass the resolved key directly to retrieve_from_store():

let key = match extract_hash(hash) {
Some(h) => h,
None if is_valid_hash(hash) => hash,
None => return tool_error("invalid stash hash: ..."),
};
// pass key directly, no re-extraction needed

7. 🔵 Error messages disclose internal filesystem paths to the LLM

src/tokenless/crates/tokenless-cli/src/mcp.rs — retrieve() function

Error strings like "stash unavailable: cannot open stash db at /home/user/.tokenless/stash.db: " are returned as MCP tool results, which the LLM sees and may echo to the end user or include in shared transcripts. This leaks the username, home directory
structure, and database implementation details.

Consider wrapping errors in generic messages for the tool result and logging the detailed error to stderr.

Review: feat(tokenless): add MCP tokenless_retrieve server

This PR adds a minimal MCP stdio server (tokenless mcp serve) that exposes a single tokenless_retrieve tool, allowing MCP-connected agents to recover payloads stashed during compress-response. The implementation is a hand-rolled JSON-RPC loop with no SDK dependency,
handling initialize, ping, tools/list, and tools/call. The stash store is opened once at startup (fail-open: if the DB is unavailable, the server still serves the protocol handshake and returns clear tool errors on retrieve). 6 unit tests cover dispatch, validation,
round-trip, and marker-line extraction.

No correctness bugs survived verification. The validation flow, hash extraction, fail-open lifecycle, and MCP response shapes are all correct. One minor finding:

1. mcp.rs:149–152 + mcp.rs:172 — extract_hash is called twice on the same input: once in retrieve() for validation, again in retrieve_from_store() for extraction. The first call validates the format (rejecting invalid input early), and the second re-extracts the key
   for the actual store lookup. Threading the extracted key through (e.g. passing key: &str to retrieve_from_store instead of the raw hash) would avoid the redundant scan. Cost: one extra linear scan of the input string per retrieve — negligible, but the code could be
   slightly tighter.

// Current: validate then re-extract
retrieve(args, store) {
if extract_hash(hash).is_none() && !is_valid_hash(hash) { ... } // call 1
retrieve_from_store(store, hash)
}
retrieve_from_store(store, hash) {
let key = extract_hash(hash).unwrap_or(hash); // call 2 (same work)
store.retrieve(key)
}

// Tighter: extract once, pass the result
let key = extract_hash(hash).unwrap_or(hash);
// validate key, then pass key directly

Clean to merge once dependency #1270 lands.

LGTM